Skip to content

Document that deploy_component waits for restart: true and reports restart_completed - #638

Merged
Ethan-Arrowood merged 7 commits into
mainfrom
kris/2335-restart-completed
Sep 2, 2026
Merged

Document that deploy_component waits for restart: true and reports restart_completed#638
Ethan-Arrowood merged 7 commits into
mainfrom
kris/2335-restart-completed

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 26, 2026

Copy link
Copy Markdown
Member

Documents the deploy_component restart behavior that HarperFast/harper#2341 changes: "restart": true now waits for the worker restart before responding, so a successful response means every worker thread is serving the new code. drop_component waits the same way.

The old behavior was undocumented and load-bearing for callers: the operation reported success while the rolling restart was still running, so a client acting on that success could be served by a worker that had never loaded the component (harper#2335).

For the human reviewer

  1. The version badges say v5.3.0, per Kris's call — the change ships in the next minor rather than a 5.2 patch. Per AGENTS.md this still gets re-checked at merge time.
  2. Placement. I put it in a new #### Restarting (restart) subsection under deploy_component, next to the existing credentials subsection, rather than expanding the one-line mention in the intro paragraph. The alternative is a shorter note inline; the subsection wins if the restart_completed semantics are worth spelling out for CI callers, which is the audience learn/developers/deploying-from-ci.mdx targets.

Verification

npm run format:write then npm run format:check — clean. Claims checked against the implementation: the awaited restart and its give-up conditions in components/operations.js and components/awaitRestart.ts, and the "rolling" path's replicated restart_service job (it does restart peers — an earlier draft of this text said it did not). An earlier revision of this PR documented a restart_completed response field; that field was dropped from #2341, and so from here.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds documentation for the restart parameter under the Deployment Operations section in reference/operations-api/operations.md, detailing the behavior of `

Comment thread reference/operations-api/operations.md Outdated
Comment thread reference/operations-api/operations.md Outdated
@github-actions
github-actions Bot temporarily deployed to pr-638 August 26, 2026 12:17 Inactive
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-638 August 26, 2026 12:26 Inactive
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638

This preview will update automatically when you push new commits.

@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-638 August 26, 2026 12:55 Inactive
@github-actions
github-actions Bot temporarily deployed to pr-638 August 27, 2026 15:06 Inactive
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-638 August 27, 2026 15:38 Inactive
@github-actions

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638

This preview will update automatically when you push new commits.

@kriszyp
kriszyp marked this pull request as ready for review September 2, 2026 03:13
@kriszyp
kriszyp requested a review from a team as a code owner September 2, 2026 03:13

@Ethan-Arrowood Ethan-Arrowood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I verified every claim in the diff against harper origin/main and they all hold:

  • restart: true waits before responding — components/operations.js:809-811 awaits awaitRestart(...)
  • Progress-based rather than fixed-timeout — awaitRestart.ts's own header says exactly that
  • The three give-up conditions map to real RestartOutcome fields (stalled, the ceiling, workersKeptOnOldCode/replacementsNotStarted)
  • The outcome is logged — logRestartOutcome(restart, ...) at :814
  • A failed restart genuinely does not fail the deploy — the result is not thrown on, and execution proceeds to the success message
  • "rolling" starts a replicated: true restart_service job and returns restartJobId (:818-826)
  • drop_component waits the same way — second awaitRestart call site at :1343
  • restart_completed is correctly absent: zero occurrences anywhere in harper

On the badge: components/awaitRestart.ts does not exist at v5.2.6, v5.2.7 or v5.2.8, so the change is merged but unreleased and v5.3.0 is defensible. (Checked by testing for the file at each tag rather than --contains, and dereferencing with ^{}git ls-remote refs/tags/v5.2.8 reports the annotated tag object 85fbf969e, not the commit 23264cebb, which is an easy way to conclude a phantom divergence.)

Two non-blocking notes, neither worth holding this up for.

The two bounds are exported constants but not documented. awaitRestart.ts exports RESTART_IDLE_TIMEOUT_MS = 60_000 and RESTART_WAIT_CEILING_MS = 600_000. The text says the response "can take as long as the install plus the restart — tens of seconds on a slow install with many worker threads" and advises that a caller with a short request timeout should use rolling instead. That advice is right, but "tens of seconds" anchors an order of magnitude below the real ten-minute ceiling, and "short" is undefined when there is an exact number available. A CI author sizing a client timeout from this could pick 60s and get cut off partway through a legitimately slow restart — the same failure this change prevents, just relocated from the server to the client. Worth stating both numbers in a follow-up.

The title is stale. It still says "and reports restart_completed", which the body correctly explains was dropped from #2341 and therefore from here. I am correcting it on the squash so it does not land in the commit message.

I also updated the branch before merging — it was 32 commits behind, and operations.md had moved in five of them, so CI had passed against a stale base.

sent with Claude Opus 5

@Ethan-Arrowood
Ethan-Arrowood merged commit c2f80c8 into main Sep 2, 2026
9 of 10 checks passed
@Ethan-Arrowood
Ethan-Arrowood deleted the kris/2335-restart-completed branch September 2, 2026 14:52
@github-actions
github-actions Bot temporarily deployed to pr-638 September 2, 2026 14:52 Inactive
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-638

This preview will update automatically when you push new commits.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🧹 Preview Cleanup

The preview deployment for this PR has been removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants